Native interop rewrite#13462
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Removed placeholder for diagram and extra line breaks.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive documentation and guides for direct native interop in Flutter, including new pages on using jnigen for Android, ffigen for iOS, and a comparison of platform integration options. The review feedback identifies several critical issues in the code snippets, such as a guaranteed runtime crash in the iOS FFI example due to incorrect string casting, missing configuration classes in the jnigen setup causing compilation failures, multiple JNI local reference memory leaks in the Android permissions example, duplicated Groovy dependencies, and syntax errors from missing closing braces. Addressing these issues will ensure the code examples are safe, correct, and idiomatic.
| classes: [ | ||
| // provided by Android OS | ||
| 'android.os.Bundle', | ||
| 'android.content.Intent', | ||
| 'android.content.Context' | ||
| ], |
There was a problem hiding this comment.
The custom SecondActivity Kotlin class is missing from the classes list in tool/jnigen.dart. Without adding it here, jnigen will not generate the Dart bindings for SecondActivity, causing the Dart compilation to fail on line 125 when referencing native.SecondActivity.type.jClass.
| classes: [ | |
| // provided by Android OS | |
| 'android.os.Bundle', | |
| 'android.content.Intent', | |
| 'android.content.Context' | |
| ], | |
| classes: [ | |
| // provided by Android OS | |
| 'android.os.Bundle', | |
| 'android.content.Intent', | |
| 'android.content.Context', | |
| 'com.example.android_launch_activity.SecondActivity' | |
| ], |
|
|
||
| // Context.fromReference ensures we get Android Context object | ||
| // rather than the default `JObject` | ||
| var context = native.Context.fromReference(Jni.androidApplicationContext.reference); |
There was a problem hiding this comment.
Instead of manually accessing the low-level .reference property and calling fromReference, you can use the more idiomatic .as() extension method on JObject to cast the context to native.Context. This matches the modern style used in jnigen.md.
| var context = native.Context.fromReference(Jni.androidApplicationContext.reference); | |
| var context = Jni.androidApplicationContext.as(native.Context.type); |
| int checkAndRequestPermission( | ||
| JObject context, | ||
| String permission, | ||
| Function callback, | ||
| ) { | ||
| // Do I have permission? | ||
| if (ContextCompat.checkSelfPermission(context, permission.toJString()) == | ||
| PackageManager.PERMISSION_GRANTED) { | ||
| callback(); | ||
| } else if (ActivityCompat.shouldShowRequestPermissionRationale( | ||
| Jni.androidActivity(PlatformDispatcher.instance.engineId!), | ||
| permission.toJString(), | ||
| ) == | ||
| true) { | ||
| // Has the user denied the permission before? | ||
| // Give a reason why I need the permission | ||
| // and allow a re-request | ||
| print("I should ask for permission"); | ||
| // TODO Flow to show UI to reshow perms dialog | ||
| return -2; | ||
| } else { | ||
| // Ask for permission | ||
| ActivityCompat.requestPermissions( | ||
| Jni.androidActivity(PlatformDispatcher.instance.engineId!), | ||
| JArray.of(JString.type, [permission.toJString()]), | ||
| 0, | ||
| ); | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
In checkAndRequestPermission, permission.toJString() is called up to three times, creating multiple JNI local references that are never released. It is much more efficient and leak-safe to convert the string once, use the variable, and release it in a finally block.
int checkAndRequestPermission(
JObject context,
String permission,
Function callback,
) {
final jPermission = permission.toJString();
try {
// Do I have permission?
if (ContextCompat.checkSelfPermission(context, jPermission) ==
PackageManager.PERMISSION_GRANTED) {
callback();
} else if (ActivityCompat.shouldShowRequestPermissionRationale(
Jni.androidActivity(PlatformDispatcher.instance.engineId!),
jPermission,
) ==
true) {
// Has the user denied the permission before?
// Give a reason why I need the permission
// and allow a re-request
print("I should ask for permission");
// TODO Flow to show UI to reshow perms dialog
return -2;
} else {
// Ask for permission
ActivityCompat.requestPermissions(
Jni.androidActivity(PlatformDispatcher.instance.engineId!),
JArray.of(JString.type, [jPermission]),
0,
);
}
return 0;
} finally {
jPermission.release();
}
}|
@jwill this is draft (v2) of the Interop rewrite. I still need to test the ffigen tutorial for iOS. I could use some extra attention on the table to help devs choose an Interop solution: https://github.com/flutter/website/pull/13462/changes#diff-39b4cf53fbefc7c1e7d95424eaea65acb9a711a8a0473ed816ff4474123220a6R15 |
|
Visit the preview URL for this PR (updated for commit b52b94c): https://flutter-docs-prod--pr13462-native-interop-rewrite-je6br604.web.app |
|
I'm iterating on the ffigen tutorial but I can't get the generated code to work with the example code. I might have to rework this tutorial to use a much simpler example. |
|
Thanks for closing that old PR!!! Once the code changes have been made and approved (by both Gemini and a DRE). I'm good! |
b52b94c to
ecc2329
Compare
|
Staged preview of the updated docs.flutter.dev site (updated for commit 52f63ad): https://flutter-docs-prod--docs-pr13462-native-interop-rewrite-wju7kso6.web.app |
Remove duplicated dependencies block Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Staged preview of the updated flutter.dev site (updated for commit 52f63ad): https://flutter-dev-230821--www-pr13462-native-interop-rewrite-h2p0zkrz.web.app |
Fixed link that was preventing dash_site from checking the links.
Added redirect for the "call-jetpack-apis" page to "jnigen" and verified with check links.
It is based on the ios calendar demo in flutter/demos
Updates #13115